Remove classes from extractor and preprocessing __init__#3898
Conversation
|
I'll fix the other imports :) I should have know we were using classes internally rather than the functions.... I just tested locally with importing extractors and preprocessors. But let me know if people are okay with this before I make a million more tweaks :) |
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
There are many things that I don't remember about the importing system so my comments are questions more than requests.
Suggestions:
I think if we want to make change easier for people that relied on the class names we can create something like the extractor_list but with a more explicit names
extractor_classees?
So instead of:
from spikeinterface.extractors import AnExtractror`they do:
from spikeinterface.extractors.extractor_classes import AnExtractorSeems like an easy change to make that would also allow us to keep the privilege naming that you guys want (the read_extractor functions) as the unique access at spikeinterface.extractors.
| NwbRecordingExtractor, | ||
| NwbSortingExtractor, | ||
| NwbTimeSeriesExtractor, | ||
| NwbTimeSeriesExtractor, # @Heberto/Alessio what do we do with this |
There was a problem hiding this comment.
make a class read_nwb_timeseries or something like that?
Or what is the question?
There was a problem hiding this comment.
I never use these functions so I just wasn't sure if I should make a read_nwb_timeseries so that answers that question. The other question was is that an event extractor or something else? and should I just list read_nwb for all of them or should I give each one separately?
There was a problem hiding this comment.
no, is a recording extractor for different type of data.
Also, do not listen read_nwb, I don't think it behaves like all those other functions and is not well maintained.
There was a problem hiding this comment.
Should I not be using read_nwb?? It's what I use to test that our labs nwb files are working as expected.
There was a problem hiding this comment.
sounds good from my end. I'll expose read_nwb but won't preference. :) Best of luck Chris :)
I like this. I would be happy to change the file name and switch to extractor classes to make this clear as long as @alejoe91 and @samuelgarcia are okay with that :) |
alejoe91
added
refactor
* Heberto discussion for nwb_timeseries * Alessio for how the full_dict should work
for more information, see https://pre-commit.ci
hopefully I fixed all the files, but the CI will tell me.
zm711
left a comment
zm711
left a comment
There was a problem hiding this comment.
missing extractor class change
for more information, see https://pre-commit.ci
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
I ran the tests suite on neuroconv and it is working. Let me know when it is ready so I can run them again.
| # the warning and then returns the "function" version which will look the same | ||
| # to the end-user | ||
| # to be removed after version 0.105.0 | ||
| def __getattr__(extractor_name): |
There was a problem hiding this comment.
Amazing, you are doing very sophisticated stuff. Maybe too much for me but I really admire.
There was a problem hiding this comment.
It was a pain to get working. But Heberto makes a great point that we don't want to pull the rug out from under our devs who build off of spikeinterface. So we warn them for a couple versions then wee can clean this all up.
|
Vous êtes tous incroyables! |
|
Been using the preprocessing imports from this PR when developing #3438 and they work a treat. So all good from me :) |
|
Last thing we need is for @h-mayorquin to run neuroconv testing suite one more time to make sure I didn't make a mistake when converting the nested dict. Then once testing is fixed I think I'm done with this! |
|
LGTM! You can merge when you want @h-mayorquin @zm711 |
|
OK, for me. |
5 participants
I went with the hard ones to start with hahah. I figured a lot of the other ones would be easier to do. Let me know what you think about this strategy.
For preprocessors this changes nothing-- it just ensures that the class and function are lined up and then only imports the functions. Example here:
For extractors this is much harder and makes one potentially controversial change. When we convert from class to function we were keeping the name of the class (in the python sense not in a class attribute sense, but to make this work we need to change the class name to be the
read_functionname. Since we want our users to only see the function I think this should be fine, but maybe someone is super against this. Or maybe we make the wrapper function a bit more complex like for the dict example that @chrishalcrow did? I think keeping the name is probably better for provenance actually.Small example here:
